-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
APPLE: OCIO 2.1.1 with native Metal support #1936
Conversation
0a20ded
to
7090205
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great contribution! As it is removing support of OpenColorIO 1.x, if this is approved, I guess we should also ask for a minimal version 2 when looking for OpenColorIO in CMake find_package()
call.
#endif | ||
|
||
if (!_looksOCIO.empty()) { | ||
transform->setDisplay(_looksOCIO.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks cannot be specified in setDisplay()
, if we want to keep the look override feature, we can use LegacyViewingPipeline pass it the DisplayTransform and use setLooksOverrideEnabled()
/ setLooksOverride()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see that there is an excellent example of this in the OCIO docs pages. I'll put the change in.
afe0184
to
1f82175
Compare
Tested on CentOS 7 and it worked nicely. I had to comment out the Metal language part because I was compiling with OCIO 2.0.4, which brings me to raising again the question about VFX Reference platform compatibility. I read not so long ago USD was just in the process of moving to VFX 2020 as the minimum supported platform. This still feature OCIO 1.1.1 which this PR drops support for, this PR actually needs VFX 2022 for Metal support which could easily be downgraded to VFX 2021 by having a conditional path for Metal. But actually supporting VFX 2020 would require re-introducing the conditional path that were present before the changes in this PR. That's something for USD leads to answer I assume. Another point, unrelated to this PR, I found the new UI control for OCIO display view selection hard to understand, I'm not exactly sure what's going on but it will probably need another look. |
Filed as internal issue #USD-7476 |
Yes, this PR will need to be reworked to *not* drop support for OCIO 1.x,
or we will not even be able to build with it internally to Pixar.
…--spiff
On Mon, Jul 11, 2022 at 10:12 AM Sunya Boonyatera ***@***.***> wrote:
Filed as internal issue #USD-7476
—
Reply to this email directly, view it on GitHub
<#1936 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABPOU2ASVICZ2B3BF3AZV2LVTRIXZANCNFSM524GCG7A>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
@spiffmon The OCIO v1 has limited GPU support (i.e. LUT approximation in GPU), no Metal support and OCIO v2 is fully backward compatible. But just out of curiosity, could you explain why using v2 is a blocker for Pixar? |
@hodoulp , Pixar does not currently have OCIO 2.x installed in our software suite, and we do have code outside of the USD project (that must cohabitate with USD) that uses OCIO. For sure we could (and plan to eventually) update to using 2.x everywhere, and if as you say 2.x is 100% backwards compatible, that should be straightforward. But it is still a reasonable-sized task, and we would not want the acceptance of this PR to be predicated on us doing that work. Additionally, as already mentioned, we are trying to be good Platform citizens to the degree possible, and build_usd.py currently tracks CY2020 except where absolutely incompatible on specific platforms. We're OK with accepting support for 2.x alongside 1.x in the code and having build_usd.py pull in 2.x only on Mac for the Metal support. We understand that switching fully to 2.x will considerably simplify parts of build_usd.py, but we can't justify it quite yet. We have verified that we cannot build against 1.1.1 with the PR as it stands currently. |
Hi Spiff, we'll update the PR to include 1.1.1 support. Is it acceptable to have this code in blocks protected by |
@jonny-apple , thank you - a smallish number of |
@spiffmon , we've re-introduced OCIO 1.1.1. But we need a mechanism to control which version of OCIO is downloaded by |
Thanks for asking, @jonny-apple ! Though it may eventually make sense to provide a switch in advance of vfxreference adoption, we'd like to be complexity-conservative for now, so would request you predicate the version on MacOS. Thanks again! |
Thanks @spiffmon , I'll make the change. |
I have a concern about having two different versions depending of the platform (by default, OCIO v1 for Windows and Linux, and OCIO v2 for macOS) because I think it could add confusion to users/devs/maintainers. The OCIO v2 added numerous new features and greatly improved GPU (and CPU) processing. So, an OCIO v1 configuration file will work great with the OCIO v2 library (i.e. macOS) but the other way around is not true (i.e. an OCIO v2 configuration file does not work with the OCIO v1 library (i.e. Windows & Linux)). I suggest to have a compilation flag (applicable to all platforms) to select the OCIO version which could be set to take OCIO v1 by default for now i.e. leaving Pixar ecosystem safe. |
926e6c7
to
2ff1574
Compare
065af35
to
8c71dc5
Compare
8c71dc5
to
51061f4
Compare
Thanks, it sounds like a really good change. I'll put it in. |
51061f4
to
561c419
Compare
I’ve updated the branch to handle both OCIO 1.1.0 or 2.1.1 based on the user selection in build_usd.py, with defaults to 1.1.0 on Linux and 2.1.1 elsewhere. I’ve moved the version-specific code in colorCorrectionTask.cpp into separate functions because the inline blocks of #ifdef made it very hard to understand the flow of the main functions. Hope this helps, Jon |
c3e18a1
to
dad22de
Compare
dad22de
to
3fdb037
Compare
7ef0117
to
3ae7705
Compare
3ae7705
to
6353992
Compare
6353992
to
01b1bcf
Compare
a902747
to
608dce6
Compare
9761d44
to
35ce01d
Compare
08ebfd3
to
ee623bd
Compare
ee623bd
to
2ae5730
Compare
Description of Change(s)
Add OCIO 2 with GPU path for openGL and Metal.
Thanks to the team at Apple for the work on implementing this.
Fixes Issue(s)